Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(generate): correct component path when module is generated in subfolder, and parent folder is not a module too #3916

Merged

Conversation

Meligy
Copy link
Contributor

@Meligy Meligy commented Jan 9, 2017

fixes #3255

Another attempt at using the correct component name / path when generating modules, that takes care of the special case in #3255.

Existing behaviour that this PR fixes

if you ng g module parent --routing, you can ng g module parent/child --routing.

But if src/app/parent is just a normal folder, the component generation for ng g module parent/child fails.

To test the PR:

  • Create a new project linked to the CLI from the PR branch
  • Inside the project, run ng g module normal-case --routing to ensure basic scenario is not broken
  • Then also run ng g module parent and ng g module parent/child --routing to ensure this scenario is not broken too
  • Then run mkdir src/app/parent-folder, and then ng g module parent-folder/child-module --routing to test that this PR does what it promises to do

Or alternatively look at the provided tests in the PR and existing tests, and ensure they cover all above scenarios, and are passing in CI.

Note

This is maybe the 3rd or so attempt at getting this path to work in all scenarios. We are protected by tests though, and we keep adding more tests the more scenarios we get. Let's see if this becomes the last change of this path/name.

@Meligy Meligy force-pushed the fix/module-component-subfolder-non-module branch 2 times, most recently from e9da693 to b4e3266 Compare January 9, 2017 15:20
@filipesilva filipesilva requested a review from Brocco January 9, 2017 15:25
@Meligy Meligy force-pushed the fix/module-component-subfolder-non-module branch from b4e3266 to 4369fd2 Compare January 9, 2017 15:49
@hansl
Copy link
Contributor

hansl commented Jan 9, 2017

Could you also add an e2e test? Acceptance tests are good but likely to be removed after 1.0 but e2e will remain.

@hansl hansl assigned hansl and unassigned hansl Jan 9, 2017
@hansl hansl self-requested a review January 9, 2017 19:44
Copy link
Contributor

@hansl hansl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, all that's missing would be an additional e2e test, as noted above.

@Meligy Meligy force-pushed the fix/module-component-subfolder-non-module branch from 4369fd2 to 4bd2bc6 Compare January 10, 2017 20:59
@Meligy
Copy link
Contributor Author

Meligy commented Jan 10, 2017

@hansl Added e2e test

@Meligy Meligy force-pushed the fix/module-component-subfolder-non-module branch 3 times, most recently from 00e79c5 to ccfaf9d Compare January 12, 2017 21:11
@Meligy
Copy link
Contributor Author

Meligy commented Jan 12, 2017

e2e test is passing now.

@hansl
Copy link
Contributor

hansl commented Jan 12, 2017

LGTM. Need @Brocco's approval.

// as `this.dynamicPath.dir` will be the same as `this.dynamicPath.appRoot`
// 2. If it does have `/` (like `parent/mod-name`), it'll be `/parent/mod-name/mod-name`
// as `this.dynamicPath.dir` minus `this.dynamicPath.appRoot` will be `/parent`
var dmouleDir =
Copy link
Contributor

@Brocco Brocco Jan 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo? rename please.

dmouleDir -> moduleDir

Also, please rebase and then it can be merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

…folder, and parent folder is not a module too

fixes angular#3255
@Meligy Meligy force-pushed the fix/module-component-subfolder-non-module branch from ccfaf9d to 512d53a Compare January 15, 2017 03:50
@Meligy
Copy link
Contributor Author

Meligy commented Jan 15, 2017

Can we mark this for merge now?

Cheers,

@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

@Brocco Brocco merged commit f70feae into angular:master Jan 16, 2017
MRHarrison pushed a commit to MRHarrison/angular-cli that referenced this pull request Feb 9, 2017
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Creating a module deeper than src/app throws Invalid path
5 participants